Updated Packages & Functionality - #111
Conversation
Updated to latest SDK's and builds
mitchelsellers
added
the
dependencies
|
Caution Review failedThe pull request is closed. WalkthroughUpdates CI/CD workflows and GitVersion configuration, bumps multiple NuGet dependencies, introduces a release changelog workflow, refactors spreadsheet generator and parser for resource and control-flow changes, and removes the serialization constructor from SpreadsheetParserException. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Parser as OpenXmlSpreadsheetParser
participant Worksheet
participant SST as SharedStringTablePart
Caller->>Parser: GetCellValue(cell)
alt DataType == SharedString
Parser->>Worksheet: Resolve parent worksheet
Worksheet-->>Parser: worksheet ref
Parser->>SST: Get string by index
SST-->>Parser: string value
Parser-->>Caller: return string
else DataType == Boolean
Parser-->>Caller: return "FALSE" if "0" else "TRUE"
else Other
Parser-->>Caller: return raw value
end
sequenceDiagram
participant Code
participant Doc as SpreadsheetDocument(IDisposable)
participant Resources
Code->>Doc: using var doc = Create(...)
activate Doc
Code->>Doc: perform operations (write sheets/cells)
Code-->>Doc: scope exit
deactivate Doc
Doc->>Resources: Dispose() — close & free resources
Note over Resources: Disposal now implicit via using var
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NetCore.Utilities.Spreadsheet/NetCore.Utilities.Spreadsheet.csproj (1)
28-30: Critical: Malformed MSBuild condition syntax.Line 28 has a syntax error:
'$GITHUB_ACTIONS)'is missing the opening$and parenthesis. This will prevent the project from building.Apply this diff:
- <PropertyGroup Condition="'$GITHUB_ACTIONS)' == 'true'"> + <PropertyGroup Condition="'$(GITHUB_ACTIONS)' == 'true'">
🧹 Nitpick comments (3)
src/NetCore.Utilities.Spreadsheet/SpreadsheetColumnAttribute.cs (1)
14-23: Constructor XML doc now correctly documentsformulaparameterThe added
<param name="formula">XML comment is accurate and brings the constructor docs in line with the existingFormulaproperty. If you want to go a step further, you could mirror the “SUM, MIN, MAX, etc.” examples from the property doc here for extra clarity, but that’s purely optional.src/NetCore.Utilities.Spreadsheet/OpenXmlSpreadsheetGenerator.cs (1)
49-90: Usingusing varforSpreadsheetDocumentis correct; consider avoiding the null-forgiving operator.The switch to
using var spreadsheetDocument = SpreadsheetDocument.Create(...)in both single- and multi-sheet writers gives proper deterministic disposal, making explicitClose()calls unnecessary and keeping the lifetime scoped to the method body. That looks good.In the multi-sheet path (Line 162), you can avoid
WorkbookPart!and rely on the already-initializedworkbookPartvariable:- var sheets = spreadsheetDocument.WorkbookPart!.Workbook.AppendChild(new Sheets()); + var sheets = workbookPart.Workbook.AppendChild(new Sheets());This keeps nullability clearer without changing behavior, since
workbookPartis assigned just above.Also applies to: 159-205
src/NetCore.Utilities.Spreadsheet/OpenXmlSpreadsheetParser.cs (1)
158-179: Shared-string resolution works but is heavier than needed; consider reusing known context.The new logic correctly:
- Walks up the parent chain to find the owning
Worksheet.- Uses
Worksheet.WorksheetPart.OpenXmlPackageandGetPartsOfType<SharedStringTablePart>()to resolve the shared string table.- Maps the numeric index in
cell.InnerTextto the actual string.Two possible improvements:
You already have
SpreadsheetDocument excelDocandWorksheetPart wsPartinParseDocumentInternal. Passing aSharedStringTablePart(or a lookup delegate) intoGetCellValuewould avoid walking the DOM and resolving the package for every shared-string cell, which could be a noticeable win on large worksheets.When
sstPartisnull, you currently fall back to returningvalue(the numeric index). If this scenario is considered exceptional rather than valid, you might prefer throwing aSpreadsheetParserExceptionso callers don’t silently receive the index instead of the string.Neither change is strictly required, but both would make behavior and performance more predictable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/release.yml(1 hunks).github/workflows/ci-build.yml(2 hunks).github/workflows/release-build.yml(1 hunks)GitVersion.yml(0 hunks)samples/NetCore.Utilities.SpreadsheetExample/NetCore.Utilities.SpreadsheetExample.csproj(1 hunks)src/NetCore.Utilities.Spreadsheet.Tests/NetCore.Utilities.Spreadsheet.Tests.csproj(1 hunks)src/NetCore.Utilities.Spreadsheet/NetCore.Utilities.Spreadsheet.csproj(1 hunks)src/NetCore.Utilities.Spreadsheet/OpenXmlSpreadsheetGenerator.cs(3 hunks)src/NetCore.Utilities.Spreadsheet/OpenXmlSpreadsheetParser.cs(1 hunks)src/NetCore.Utilities.Spreadsheet/SpreadsheetColumnAttribute.cs(1 hunks)src/NetCore.Utilities.Spreadsheet/SpreadsheetParserException.cs(0 hunks)
💤 Files with no reviewable changes (2)
- GitVersion.yml
- src/NetCore.Utilities.Spreadsheet/SpreadsheetParserException.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/NetCore.Utilities.Spreadsheet/OpenXmlSpreadsheetParser.cs (1)
src/NetCore.Utilities.Spreadsheet/SpreadsheetParserException.cs (2)
SpreadsheetParserException(12-15)SpreadsheetParserException(17-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze Code Quality
🔇 Additional comments (8)
src/NetCore.Utilities.Spreadsheet/NetCore.Utilities.Spreadsheet.csproj (1)
37-42: Based on my verification, I can now provide the rewritten review comment:
DocumentFormat.OpenXml 3.3.0 is compatible with the spreadsheet parsing and generation code.
The codebase uses high-level OpenXml APIs (SpreadsheetDocument, Descendants(), Elements(), Append(), GetPartsOfType(), etc.) that remain stable in 3.x. The breaking changes between 2.16.0 and 3.x (such as OpenXmlPackage.Package type change to IPackage, OpenXmlElementList becoming a struct, and TypedOpenXmlPartReader removal) do not apply to this implementation. The code was designed for 3.3.0 and compiles successfully with the target framework net9.0.
src/NetCore.Utilities.Spreadsheet.Tests/NetCore.Utilities.Spreadsheet.Tests.csproj (1)
10-21: Let me verify the target framework and check for any potential compatibility issues:
<function_calls>
#!/bin/bashCheck the test project's target framework
echo "=== Test Project File (lines 1-30) ==="
head -30 src/NetCore.Utilities.Spreadsheet.Tests/NetCore.Utilities.Spreadsheet.Tests.csprojecho -e "\n=== Main Project Target Framework ==="
head -30 src/NetCore.Utilities.Spreadsheet/NetCore.Utilities.Spreadsheet.csproj | grep -i targetCheck for using statements in test files
echo -e "\n=== Using Statements in Test Files ==="
rg -n '^using ' src/NetCore.Utilities.Spreadsheet.Tests --type cs -m 10
</blockquote></details> <details> <summary>.github/workflows/ci-build.yml (3)</summary><blockquote> `81-88`: **Consistent GitVersion upgrade across jobs.** The GitVersion v4.2.0 upgrade is consistently applied. Ensure the verification from the build job above covers both jobs. --- `91-100`: **Security improvement: Sonar token passed via environment variable.** The change to use `env.SONAR_TOKEN` instead of inline secrets reference is a security best practice. Secrets passed via environment variables are less likely to leak in logs or debug output. --- `26-33`: **No compatibility issues found. The upgrade to GitVersion action v4.2.0 is safe and compatible with the project.** The GitVersion.yml configuration exists at the repository root with standard settings that v4.2.0 will auto-detect. The `useConfigFile` parameter removal in v4 indicates the action now auto-discovers the configuration file, which is the case here. The versionSpec 6.4.0 (minor bump from 6.0.5) is compatible, and the `majorMinorPatch` output variable referenced in the workflow (line 38) is available in v4.2.0. No configuration changes are required. </blockquote></details> <details> <summary>.github/workflows/release-build.yml (1)</summary><blockquote> `25-32`: **GitVersion v4.2.0 upgrade aligned with CI workflow.** Release workflow applies the same GitVersion v4.2.0 and versionSpec 6.4.0 updates as the CI build workflow, ensuring consistency across pipelines. The verification needed for ci-build.yml applies here as well. </blockquote></details> <details> <summary>.github/release.yml (1)</summary><blockquote> `1-23`: **Changelog configuration looks well-structured.** The release configuration provides clear categorization of PRs into changelog sections and excludes non-relevant issue types (question, wontfix, invalid). The catch-all "Other Changes" category ensures comprehensive coverage. This is a solid foundation for automated release notes. </blockquote></details> <details> <summary>src/NetCore.Utilities.Spreadsheet/OpenXmlSpreadsheetParser.cs (1)</summary><blockquote> `181-191`: **Clarify boolean cell semantics and ensure tests cover them.** The boolean branch now returns `"FALSE"` only for `"0"` and `"TRUE"` for any other value: ```csharp return value switch { "0" => "FALSE", _ => "TRUE" };This is simple, but it means any unexpected value (e.g.,
"2"or"foo") will also be treated as"TRUE". If that’s intentional, it would be good to have tests documenting it; if not, you may want a stricter mapping ("0"/"1"only, or explicit error for invalid values).
….SpreadsheetExample.csproj Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
1 participant
Updated to latest SDK's and builds
Summary by CodeRabbit
Chores
Refactor
Bug Fix
✏️ Tip: You can customize this high-level summary in your review settings.